-
Notifications
You must be signed in to change notification settings - Fork 4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Develop #4557
base: master
Are you sure you want to change the base?
Develop #4557
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issues with the @font-face rule and margin property need to be addressed as they could impact the styling and layout consistency. Please make these adjustments for a more robust implementation. Keep up the great work! 😊
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
src/style.css
Outdated
@font-face { | ||
font-family: Avenir-Book; | ||
src: url(./fonts/Avenir-Book.ttf); | ||
font-family: Avenir-Heavy; | ||
src: url(./fonts/Avenir-Heavy.ttf); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The @font-face rule is incorrectly structured. Each font-face declaration should be separate. You should not define multiple font-family and src properties within a single @font-face block.
} | ||
|
||
body { | ||
margin: 0 8px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The margin property should be consistent with the checklist recommendation to avoid potential margin collapse. Consider using only top or bottom margin, but not both.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job on structuring the HTML file! 🎉 While there are minor issues with indentation, spacing, and font fallbacks, these are excellent learning opportunities. Please review the comments to enhance your code. Keep up the good work and continue improving!
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
} | ||
|
||
body { | ||
margin: 0 8px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider using only top or bottom margin to avoid potential margin collapse. The checklist suggests being consistent with your margins.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have couple comments here:
-
There is nothing stated in the task to set horizontal margins, I had to ask mentor to understand why tests were failing and he suggested putting "margin: 0 10px", but at 10px tests still were failing. I had to find the correct margin to comply the tests. I believe this point should be added to task description or tests should be updated
-
Why the bot says it's incorrect margins? I was suggested by mentor to set margins this way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@etojeDenys please check these questions
border: 1px solid #e1e7ed; | ||
font-weight: 300; | ||
box-shadow: 0 1px 8px 0 #3d4e611a; | ||
font-family: Avenir-Heavy, sans-serif; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a good practice to provide fallback fonts. Consider adding a generic font family like 'sans-serif' as a fallback for 'Avenir-Heavy'.
} | ||
|
||
.input::placeholder { | ||
font-family: Avenir-Book, sans-serif; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding a generic font family like 'sans-serif' as a fallback for 'Avenir-Book' to ensure proper rendering if the custom font fails to load.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
DEMO LINK
TEST REPORT LINK
Icon implemented using background-image CSS property
Inputs are written inside of 'form' tag with correctly passed attributes
All
Typical Mistakes
fromBEM
lesson theory are checked.Code follows all the Code Style Rules ❗️